fix(acp): defer authentication until required - #924

Signed-off-by: Matt Toohey <contact@matttoohey.com>

Rebasing `codex-broken` onto main picked up #939, which replaced the
`writer` field on `AcpSessionSetupContext` with a `handler` that owns
the writer. The eager-authentication regression test added by the
rebased "defer authentication until required" commit still passed a
bare writer, so the acp-client test target no longer compiled.

Construct an `AcpNotificationHandler` around a `BasicMessageWriter` the
same way the surrounding tests do and hand that to the context instead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

Addresses the review of 7c10819 across the ACP driver, the doctor fix
runner, and the session-pane login recovery UI.

doctor: a login fix could only ever end by finishing or by timing out, so
a pane disposed mid-login left the subprocess running and — because the
active-login map is keyed by check ID — refused every later login attempt
for that agent until the fix timed out. Adds a `FixCancel` handle built on
the same idioms as `FixStdin` (shared state machine, Drop guard, poison
recovery) plus a pid-based `kill_process_group_or_process`, so a canceller
on another thread can end a fix while the spawning thread is parked in
`wait`. A cancelled fix reports "Fix was cancelled" instead of the dying
shell's stderr. Surfaced as a `cancel_doctor_login` command on both the
Tauri and web transports, and called from `onDestroy`.

Restores the unbounded wall clock for the non-interactive install and
update fixes: they inherit stdin, so they are not in their own process
group and a firing timeout could kill only the login shell while
`npm install -g` kept running orphaned. The 600s bound stays on the
interactive login path, which does get its own process group. Also
restores the `run_doctor_fix` doc comment dropped on this branch.

acp-client: the auth-required error is rendered verbatim in the session
alert, so the advertised-method inventory moved to a debug log and the
error is now one actionable sentence. Drops the unused serde derives on
the authentication types rather than advertise a wire shape no consumer
has agreed to.

SessionChatPane: awaits listener registration (raced against a grace
timer, so a failed registration cannot hang the UI on "Logging in…")
before starting the fix, renders the streamed login output, resets the
code prompt when the fix ends, confirms success in place, fetches the
doctor report when an auth error is shown, and imports the login commands
statically. `isAuthCodePrompt` is narrowed so ordinary diagnostics like
`type: error code 401` no longer pop a code input.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

Addresses four of the five findings from the review of 7ffa2b7a. The
fifth — whether the other ACP bridges doctor knows about return
`auth_required` rather than a generic error when signed out — is being
researched separately and is not touched here.

SessionChatPane, session switch: the pane is reused across sessions,
and the branch that clears per-session state when `sessionId` changes
left every piece of login-recovery state alone, so the previous
session's login transcript, code input or "Signed in" hint rendered
under the next session's alert, and a login still running kept another
provider's Log in button disabled. That state is now reset there. An
in-flight login is cancelled on the switch, exactly as `onDestroy`
already did, rather than left to finish in the background: after the
switch there is no UI that can show its output or feed it a code, and
leaving it running would make a pane for the same provider hit "A login
is already running" until the fix timeout. The cancel-and-reset is one
`abandonLogin` helper shared by teardown and the session-change branch.
A `loginAttempt` counter lets a `startLogin` that was mid-await when the
pane moved on stay silent about its outcome, and stop the login the
backend was still starting when the earlier cancel found nothing.

SessionChatPane, listener registration: the grace timer used to start
the login anyway when the output listener had not gone live, and the
comment claimed only the first lines were at risk. When registration
had actually failed, the `done` event was missed as well, so
`loginRunning` stayed true and the button read "Logging in…" until the
pane was destroyed. The transport now reports a failed registration
through a new `onRegistrationFailed` listen option (Tauri: `listen()`
rejected; web: the socket could not be set up at all), and `startLogin`
treats either that or ten seconds without `onEstablished` as a soft
failure: the login is not started, the button is re-enabled and the
error reads "Could not subscribe to login output, try again". Starting
without a listener was dropped rather than kept with a recovery timer,
because a login nobody is listening to is not degraded but stuck: its
first lines carry the URL and device code, and its `done` is what
re-enables the button. Two transport tests cover the hook firing on a
rejected registration and staying silent after an early unlisten.

acp-client: `describe` on `AcpAuthenticationRequired` both built the
user-facing sentence and logged the method inventory as a side effect,
so a second formatting would double-log. It is pure now, and the two
call sites in `send_session_setup_request` call `log_methods` explicitly
at the point the error becomes final.

doctor: a cancel only unblocked the streaming loop indirectly — the
kill closed the shell's pipes and the readers hit EOF. A descendant that
escaped the process group while holding the inherited pipes defeated
that, so the loop waited out the full fix timeout, `done` was never
emitted and the `ACTIVE_LOGINS` entry stayed claimed for the whole wait.
While a cancel handle exists the wait now ticks every 100ms and asks the
handle directly; on an observed cancel the loop stops waiting on EOF,
kills and reaps the child the same way the timeout path already does,
and returns, which lets the login task emit `done` and release the
entry. The two interrupts share one exit path via a small
`FixInterrupt` enum. A cancelled fix that exited zero before the kill
landed still reports success, matching the EOF path. Fixes without a
cancel handle keep the plain blocking receive.

The unit test for that path was feasible: a backgrounded
`perl -MPOSIX=setsid` grandchild prints the ready line only after
`setsid()`, so the cancel provably lands on a process that has already
left the group and keeps the pipes open for 20s; the test asserts the
loop returns with the cancelled result within 5s of the cancel. It was
verified to fail (waiting the full 20s) with the poll disabled and pass
with it enabled. The elapsed bound is measured from the cancel rather
than the spawn, because a login shell can take seconds to start on a
loaded machine.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

…contract

Implements the two follow-ups from the bridge survey note (a1ba6f51),
which found that amp, claude, and goose with a native provider accept
session/new while signed out and raise ACP -32000 auth_required only at
session/prompt.

acp-client: run_prompt_turn formatted every prompt failure as
`Prompt failed: {e:?}`, so a signed-out user of those three bridges read
a nested Debug dump in the session alert where the session/new path
reads one sentence. A new describe_prompt_error matches
ErrorCode::AuthRequired and returns
AcpAuthenticationRequired::describe("run the prompt"), logging the
advertised-method inventory through log_methods at the point the error
becomes final, exactly as send_session_setup_request does. Every other
prompt error keeps its Debug rendering, and the after-cancellation
branch is untouched since `run` maps errors after a cancel to Cancelled
anyway. There is no authenticate-and-retry on the prompt path: a prompt
may already have streamed output, and the remedy is the same sign-in
either way.

To make the inventory available at prompt time, AcpSessionSetup gains an
auth_methods field copied from the initialize response, the same data
the setup path already passes to send_session_setup_request. On the
AGENTS.md rule requiring review before adding backend fields: this is a
private, in-memory struct scoped to one connection, not a persisted
model, so it was judged not to apply, as with the login token in
c9af50c8; flagging for the reviewer.

Tests: a fake agent answers session/prompt with Error::auth_required()
(asserted to be -32000) and the test asserts the exact sentence, that
it still contains "authentication", and the exact string after `run`'s
internal_error wrapping. A second test pins that a non-auth prompt error
keeps the `Prompt failed: Error { ... }` shape. The existing session/new
auth test now also asserts its exact wrapped string.

authRecovery.test.ts: pins the real strings the frontend receives, with
the `ACP protocol failed: Error { code: -32603: Internal error, message:
"Internal error", data: Some(String("...")) }` wrapper, for session/new,
session/load, the new session/prompt sentence, and, as a regression
guard, the pre-change prompt Debug shape with its escaped inner quotes.
The strings were produced by executing the driver code and its tests
rather than typed from memory, and the Rust tests assert the same bytes
so the two sides cannot drift apart silently. A negative case documents
that goose's native-provider message "Provider is not configured" is
intentionally not an authentication error: doctor has no login command
for goose, so canOfferLogin is false for it regardless and the alert can
only offer Fix.

Verification: `cargo fmt --all --check` clean; `cargo clippy -p
acp-client --tests -- -D warnings` clean; `cargo test -p acp-client` 144
passed (the doctest target hit the known shared-target-dir E0463 on the
first run and passed on re-run in isolation, 0 doctests); `pnpm check`
0 errors 0 warnings; `pnpm vitest run src/lib/features/sessions` 12
files, 203 tests passed, including the 5 new cases.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

Addresses the warning and the three suggestions from the review of
c9af50c8 and 4d9ae890; the informational comments needed no action.

Login events, the event-side race: `DoctorLoginOutput` carried no run
token, so the pane's listener accepted any `done` for
`ai-agent-<provider>`. A `done` from a run the pane had just cancelled
could land after the pane started a fresh run for the same provider,
end the new login in the UI, unlisten, and leave the start response to
record a run nobody was listening to. The next click then failed with
"A login is already running" and the `catch` nulled `activeLogin`, the
one handle that could have cancelled that run, so the provider was
locked out until the 600s fix timeout.

The token is now minted by the pane (`mintLoginToken`: crypto.randomUUID
with a time-plus-random fallback for web mode outside a secure context)
and passed to `start_doctor_login`, which rejects an empty one, stores
it on the `ACTIVE_LOGINS` entry as before, and stamps it on every
`DoctorLoginOutput`, `line` and `done`, on both transports; the command
now returns nothing. Client-side minting was chosen over "server-minted,
filter once `activeLogin` is set" because nothing orders the start
response before the run's first events: on Tauri the command reply and
emitted events travel over separate IPC paths, and in web mode the reply
is an HTTP response while events arrive over the socket. A listener that
must wait for the reply to learn its token would have to either drop the
lines that arrive first, which are the URL and the device code the user
is waiting for, or accept every line for its check ID in the meantime,
which is the race again. With the token known before anything is
listened to or started, the filter is exact from the first event.
`cancel_doctor_login` and `send_doctor_login_code` still require the
matching token, unchanged.

The routing moved into `loginOutputHandler` in authRecovery.ts, which
drops any event whose check ID or token is not the current run's and
otherwise calls `onLine`/`onDone`; the pane keeps its state changes in
those callbacks. The `catch` in `startLogin` no longer nulls
`activeLogin`: the assignment inside the `try` is its last statement, so
a throw means this attempt never recorded itself, and the null could
only discard a run an earlier attempt still owned. A closure-local
`ended` flag keeps a run whose `done` beat the start response from being
recorded as active afterwards. Four vitest cases cover the helper: a
stale-token `line` and `done` and another agent's `done` are ignored,
the current run's `line` and `done` are delivered (with and without an
error), and the minted token is non-empty and distinct per call.

Driver, the agent's own message: a `-32000` whose `message` or string
`data` says more than the stock "Authentication required" (pi-acp's
"Configure an API key or log in with an OAuth provider." at session/new)
now has that appended to the describe sentence as "The agent said: ...".
`AcpAuthenticationRequired` gains an `agent_detail` field set through
`with_agent_detail(&error)` wherever a `-32000` becomes final: both arms
of `send_session_setup_request` (the retry arm takes the second error's
detail) and `describe_prompt_error`, so setup and prompt stay
symmetrical. `agent_auth_required_detail` keeps message then data, once
each; skips the stock text (case-insensitive, trailing period ignored),
blanks, and structured data; collapses whitespace; and ends each part as
a sentence. The text still opens with "ACP authentication is required",
so `isAuthenticationError` matches, and the stock error produces exactly
the strings already pinned, so authRecovery.test.ts's pinned strings are
unchanged. Tests: a fake agent answering session/new with a custom
message asserts the full appended string; `describe_prompt_error` with
the same error asserts the prompt form; a unit test pins the stock case
unchanged and the skip, dedupe and collapse rules.

Driver, session/load pin: authRecovery.test.ts claimed every pinned
string had a byte-for-byte Rust counterpart, but nothing drove
`load_session` into `auth_required`. A new test initializes a fake agent
with `load_session: true`, answers `session/load` with `-32000`, drives
`setup_acp_session` with an existing agent session ID, and asserts the
exact wrapped string the frontend pins and that only session/load was
called. The frontend comment now names the three Rust tests it relies on.

On the AGENTS.md rule requiring review before adding backend fields: the
`token` on `DoctorLoginOutput` is a field on an ephemeral event payload
and `agent_detail` sits on a private in-memory struct; neither is a
persisted model, so the rule was judged not to apply, as with c9af50c8
and 4d9ae890. Flagging for the reviewer.

Verification: `cargo fmt --all --check` clean (workspace and src-tauri);
`cargo clippy -p acp-client -p doctor --tests -- -D warnings` clean;
`cargo test -p acp-client -p doctor`: acp-client 148 passed (4 new),
doctor 130 passed, 0 failed; `cargo check` in apps/staged/src-tauri
clean; `pnpm check` 0 errors 0 warnings; `pnpm vitest run` 73 files,
926 tests passed (4 new).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

…und-trip

Signed-off-by: Matt Toohey <contact@matttoohey.com>

Signed-off-by: Matt Toohey <contact@matttoohey.com>

Move the remaining login orchestration out of SessionChatPane into a
pane-local controller exposing checkId, canLogin, running and start.
Keep report loading, reattachment and completion refreshes together,
with the pane responsible only for rendering the recovery actions.

Build on the existing shared Doctor login controller rather than
introducing another state machine. Preserve run ownership, cancellation,
reconnect recovery and login lifetime across pane closes and session
switches; leave the backend protocol and pure authRecovery helpers alone.
Defer the note's optional background-hold ordering change to keep this
refactor behavior-preserving.

Add 35 controller tests covering eligibility, request deduplication,
reopening, session switches, shared-login exclusion and completion after
the pane closes. Verification: pnpm check (no errors or warnings), all
995 Vitest tests passing, and formatting checks clean.

Signed-off-by: Matt Toohey <contact@matttoohey.com>

Wire doctor login starts and attach probes into the transport registration-failure hook so failed listener setup re-enables the UI instead of leaving login state stuck running.

Make web event setup clear its connecting state through failure paths, notify all listeners waiting on the failed socket setup, and drop those failed listeners so later registrations can retry cleanly.

Signed-off-by: Matt Toohey <contact@matttoohey.com>

Addresses the review of 3fb0599: a failed web-socket set-up (getWsUrl
or the WebSocket constructor throwing) cleared every registered
listener, not just the ones that asked to hear about the failure. In
Tauri mode a rejected listen() costs only that one listener, but here a
single throw silently unsubscribed session status, the change feed and
every other listenToEvent/listenToWindowEvent caller registered at that
moment, with only a console.error and no way back even though the very
next registration would have reconnected fine.

notifyListenersRegistrationFailed now removes only listeners carrying
onRegistrationFailed — their owner has been told the registration is
dead and has given up on it — and leaves the rest in wsListeners for
the next ensureWebSocket to pick up, which is how they behaved before
the hook was added. No retry is scheduled for a setup failure itself;
that path is unchanged.

Adds a transport test where a plain listener and a login listener share
a failing set-up: only the login hook fires, and once a later
registration connects, the plain listener still receives its event while
the dropped login listener does not.

Verification: pnpm vitest run on transport and agentLogin suites (55
passed), pnpm check 0 errors 0 warnings, prettier clean.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>

Addresses the review of 3fb0599 on crates/doctor/src/command.rs:
after d315bde removed the pid-based canceller, `kill_pid` was left
with a single caller, `kill_child_process_group`, which only negated
the pid before handing it over. The split existed to share the raw
`kill(2)` call with the removed path and no longer explained anything.

Fold the `nix::sys::signal::kill` call back into
`kill_child_process_group` and move the doc comment onto it, so the
negative-pid-means-process-group note sits next to the one place that
negates the pid. No behaviour change; the `cfg(unix)` gating and the
`i32::try_from` guard are unchanged.

Verification: `cargo fmt --all --check` clean; `cargo clippy -p doctor
--tests -- -D warnings` clean; `cargo test -p doctor` 151 passed,
0 failed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>